Skip to content

[SYCL] Implement reduction properties extension #15804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 16 commits into from
Oct 29, 2024
Merged

[SYCL] Implement reduction properties extension #15804

merged 16 commits into from
Oct 29, 2024

Conversation

@Pennycook
Copy link
Contributor

@Pennycook Pennycook commented Oct 22, 2024 •
edited
Loading

Adds support for initialize_to_identity and deterministic properties.

Since this extension is only experimental, the implementation here avoids making significant changes to reduction-related classes (e.g, reducer).

A more straightforward implementation that attaches a compile-time property list to these classes is possible, but may be considered an ABI break.

A note to reviewers: it occurred to me that IsDeterministicOperator<BinaryOperation> might actually be better as IsDeterministicOperator<T, BinaryOperation>, so that the implementation can infer that it's safe to use the faster reductions for certain types (e.g., int and sycl::plus<>() can safely use atomics) but I wanted to get your feedback on the proposed approach before going any further.

EDIT: To clarify, I think such changes to IsDeterministicOperator could be left until a future PR, since the behavior of the deterministic property as implemented here should match the legacy SYCL_DETERMINISTIC_REDUCTION macro. We could implement optimizations later.

@Pennycook
[SYCL] Implement reduction properties extension
b94cb3f 
Adds support for initialize_to_identity and deterministic properties.

Since this extension is only experimental, the implementation here avoids
making significant changes to reduction-related classes (e.g, reducer).
A more straightforward implementation that attaches a compile-time property
list to these classes is possible, but may be considered an ABI break.

Signed-off-by: John Pennycook <[email protected]>
@Pennycook
[SYCL] Add basic tests of reduction properties
75197db 
Signed-off-by: John Pennycook <[email protected]>
@Pennycook
[SYCL][Doc] Mark reduction properties experimental
8df293c 
Also fixes a few typos in the specification that were uncovered during
implementation of the extension.

Signed-off-by: John Pennycook <[email protected]>
@Pennycook Pennycook added the spec extension All issues/PRs related to extensions specifications label Oct 22, 2024
@Pennycook
Remove operator names
94f9b70 
Signed-off-by: John Pennycook <[email protected]>
@Pennycook
Remove sycl/sycl.hpp from tests
d87de38 
Signed-off-by: John Pennycook <[email protected]>
@aelovikov-intel
Copy link
Contributor

aelovikov-intel commented Oct 22, 2024

Any chance we can be "smart" and change

template <typename T, typename BinaryOperation>
auto reduction(T *Var, const T &Identity, BinaryOperation Combiner,
               const property_list &PropList = {})

to

template <typename T, typename BinaryOperation, typename PropertyListTy = property_list>
auto reduction(T *Var, const T &Identity, BinaryOperation Combiner,
               const PropertyListTy &PropList = {})

instead of adding a bunch of new overloads?

aelovikov-intel
aelovikov-intel reviewed Oct 22, 2024
View reviewed changes
@Pennycook
Fix use of host pointer on device
7e05778 
Bug was hidden during development due to testing on the CPU.

Signed-off-by: John Pennycook <[email protected]>
@Pennycook
Copy link
Contributor Author

Pennycook commented Oct 23, 2024

Any chance we can be "smart" and change

template <typename T, typename BinaryOperation>
auto reduction(T *Var, const T &Identity, BinaryOperation Combiner,
               const property_list &PropList = {})

to

template <typename T, typename BinaryOperation, typename PropertyListTy = property_list>
auto reduction(T *Var, const T &Identity, BinaryOperation Combiner,
               const PropertyListTy &PropList = {})

instead of adding a bunch of new overloads?

I'd rather keep the overloads for now, if that's okay. Because this extension is only experimental, I don't want us to make changes to the "real" reduction interface if we don't have to. Plus, if we're successful with getting compile-time properties into a future version of the SYCL specification, I expect we'll want to generate a deprecation notice for the property_list overloads, and I can't think of a way to do that without splitting things out.

@Pennycook Pennycook marked this pull request as ready for review October 23, 2024 12:27
@Pennycook Pennycook requested review from a team as code owners October 23, 2024 12:27
@Pennycook
Copy link
Contributor Author

Pennycook commented Oct 28, 2024

@gmlueck - Could you please take a quick look at the extension specification changes here? They're just typo fixes so it hopefully shouldn't take too long to review.

@Pennycook
Copy link
Contributor Author

Pennycook commented Oct 29, 2024

@intel/llvm-gatekeepers, I think this can be merged now.

@sarnex sarnex merged commit 22e5ced into intel:sycl Oct 29, 2024
14 checks passed
@Pennycook Pennycook deleted the sycl_ext_oneapi_reduction_properties branch January 16, 2025 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmlueck gmlueck gmlueck approved these changes

@aelovikov-intel aelovikov-intel aelovikov-intel approved these changes

@steffenlarsen steffenlarsen Awaiting requested review from steffenlarsen

Assignees

No one assigned

Labels

spec extension All issues/PRs related to extensions specifications

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Pennycook @aelovikov-intel @gmlueck @sarnex